Add reduce op by guoshengCS · Pull Request #4086 · PaddlePaddle/Paddle

guoshengCS · 2017-09-13T17:14:24Z

Resolves #4060

luotao1 · 2017-09-15T09:09:10Z

paddle/operators/reduce_op.cc

我觉得将min，max等作为attr来写，注册一个kernel，这样cc文件会更加简短精炼。

目前ReduceMinOpMaker和ReduceMaxOpMaker等基本都是重复的。现在分开写了四个kernel，写成一个后，会省将近3/4的代码。

抱歉啊，这个回复的晚了，这里参考了下tensorflow的做法 https://github.com/tensorflow/tensorflow/blob/216dcbf1e08c02d87774120ebd5b251c5c30c56c/tensorflow/core/kernels/reduction_ops_sum.cc#L26 ，另外pytorch中也有类似的reduce操作 https://github.com/pytorch/pytorch/blob/master/torch/autograd/_functions/reduce.py ，感觉可能分为多个OP在意义上更清楚一些，另外将min、max作为attr感觉会kernel部分的代码会比较长，后续可能也会加下其他的reduce操作，functor的话还有一个潜在好处是的可以复用目前的kernel不太容易复用，我也不能确定哪种会更好。多谢评论与思考建议~

luotao1 · 2017-09-15T09:11:58Z

paddle/operators/reduce_op.h

这里的6个case是什么意思呢？

这里对应到EigenTensor的几种rank，由于EigenTensor是用的template，这里显示写了出来

luotao1 · 2017-09-15T09:14:01Z

paddle/operators/reduce_op.h

如果作为attr来写，这里也不用functor，用switch来做，29-86行中一半的代码都能省出来。

Functor的话还有一个潜在好处是的可能比较容易复用，目前的kernel好像不太容易复用

luotao1 · 2017-09-15T09:15:39Z

paddle/operators/reduce_op.cu

目前实现是，每个循环里调用一次eigen的gpu kernel，效率会比较慢。可以考虑单独写。

这个不是特别什么意思，可以稍具体说明下么~

gongweibao

有些代码可以省略掉

gongweibao · 2017-09-19T01:17:04Z

python/paddle/v2/framework/tests/test_reduce_op.py

定义一个父类，然后TestMeanOp(xxxx),然后test_check_out等函数就都可以重用了。

好的建议，多谢。这里reduce_max和reduce_min不进行梯度检测，几种OP中test的内容不是特别统一，感觉就先行不改了。

gongweibao · 2017-09-19T01:25:54Z

paddle/operators/reduce_op.cc

Sum,Mean,Max,Min的几个OpMaker基本都是一样的。这些代码其实可以用一个基类实现，然后子类来处理不相同的部分。
参考一下：#4139 ElementwiseOpmaker。

Done. Thanks~

gongweibao · 2017-09-19T01:31:00Z

paddle/operators/reduce_op.cc

咱们的Tensor最多支持9维。这个6维是怎么来的？

这里目前参考的是crop_op中的https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/operators/crop_op.h ，可能确实需要再统一下。

qingqing01 · 2017-09-14T04:38:09Z

paddle/operators/reduce_op.cc

ReduceMean -> ReduceSum

qingqing01 · 2017-09-25T02:32:48Z

paddle/operators/reduce_op.cc

+namespace operators {
+
+using framework::Tensor;
+using framework::LoDTensor;


remove this line.

qingqing01 · 2017-09-25T02:33:02Z

paddle/operators/reduce_op.cc

+      dims_vector.erase(dims_vector.begin() + dim);
+    }
+    auto out_dims = framework::make_ddim(dims_vector);
+    ctx.Output<framework::LoDTensor>("Out")->Resize(out_dims);


framework::LoDTensor -> framework::Tensor

qingqing01 · 2017-09-25T02:37:15Z

paddle/operators/reduce_op.cc

+    AddOutput("Out", "(Tensor) The result tensor.");
+    AddAttr<int>("dim",
+                 "(int, default 0) The dimension to reduce. "
+                 "Must be in the range [-rank(input), rank(input))")


Add more comments for the dim, or add some examples in the Doc.
这里-dim是倒数第几维吧？加下注释吧。

qingqing01 · 2017-09-25T03:28:42Z

paddle/operators/reduce_op.h

+    auto equals = x == y.broadcast(dim);
+    auto ones = dx.constant(1);
+    auto zeros = dx.constant(0);
+    dx.device(place) = dy.broadcast(dim) * equals.select(ones, zeros);


如果max/min值有多个，backward梯度传播的时候，这里的实现多个max值的梯度都为max的grad，而不是一些max值的梯度为0，另外，和 @guoshengCS 讨论，TF这里对多个max/min值的梯度，取了平均： https://github.com/tensorflow/tensorflow/blob/37f7ad75bbd2ca140d1092342eb3590d54193bc8/tensorflow/cc/gradients/math_grad.cc#L711

咱们这里的处理加下注释吧~

qingqing01 · 2017-09-25T05:09:14Z

paddle/operators/reduce_op.h

+    auto* input2 = context.Input<Tensor>(framework::GradVarName("Out"));
+    auto* output = context.Output<Tensor>(framework::GradVarName("X"));
+
+    if (output != nullptr) {


如果backward的输出只有一个的话，内部实现不用考虑output == nullptr，这种case，在https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/framework/backward.cc#L67 里会返回NOP。

qingqing01 · 2017-09-25T05:11:46Z

paddle/operators/reduce_op.h

+
+ private:
+  template <size_t D>
+  void ReduceCompute(const framework::ExecutionContext& context) const {


为了和上面ReduceCompute区别，这里是否要叫做ReduceGradCompute?

qingqing01 · 2017-09-25T05:12:56Z

paddle/operators/reduce_op.h

+
+// For EigenTensor unsupported reduce
+template <typename T, typename Functor>
+class ReduceGradEigenFreeKernel : public framework::OpKernel {


这个kernel用在哪里？

Done. 这里先行删掉

… add-ReduceOp

luotao1

LGTM.

gongweibao

LGTM

QiJune mentioned this pull request Sep 13, 2017

Implement activation related operators #4071

Merged

qingqing01 added the OpPorting label Sep 14, 2017

qingqing01 requested review from Superjomn, dzhwinter, hedaoyuan and qingqing01 September 14, 2017 04:39

luotao1 reviewed Sep 15, 2017

View reviewed changes

qingqing01 requested review from QiJune and removed request for Superjomn September 18, 2017 08:33

gongweibao requested changes Sep 19, 2017

View reviewed changes

guoshengCS added 4 commits September 24, 2017 12:44

Add reduce_op

3994e91

Revise the reduce_op unit test accordingly

c8d8771

Fix reduce_op according to CI log

630273d

Refine reduce_op and follow comments

8b3bf28

guoshengCS force-pushed the add-ReduceOp branch from ebcf710 to 8b3bf28 Compare September 24, 2017 07:52

Refine reduce_op unit test and add newline at end of file

1295e5e

qingqing01 reviewed Sep 25, 2017

View reviewed changes

Refine reduce_op, follow comments and remove ReduceGradEigenFreeKernel

477a6a0

guoshengCS force-pushed the add-ReduceOp branch from 7651339 to 477a6a0 Compare September 26, 2017 07:49

guoshengCS added 3 commits September 27, 2017 11:52

Merge branch 'develop' of https://github.com/PaddlePaddle/paddle into…

99b8dbb

… add-ReduceOp

Merge branch 'develop' of https://github.com/PaddlePaddle/paddle into…

be58c63

… add-ReduceOp

Adapt reduce_op according to up-to-date dev

e33b411

luotao1 approved these changes Sep 28, 2017

View reviewed changes

gongweibao approved these changes Sep 28, 2017

View reviewed changes

guoshengCS merged commit ecef2e6 into PaddlePaddle:develop Sep 28, 2017

Conversation

guoshengCS commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gongweibao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guoshengCS Sep 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luotao1 left a comment

Choose a reason for hiding this comment

Uh oh!

gongweibao left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

guoshengCS Sep 24, 2017 •

edited

Loading